Skip to content

Conversation

ViktorT-11
Copy link
Contributor

@ViktorT-11 ViktorT-11 commented Sep 11, 2025

Based on #1146

This PR introduces the migration logic for transitioning the actions from kvdb to SQL.

Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

Part of #917

I will cleanup the code a little bit and add some more details to the commit messages before requesting reviews.

@ViktorT-11 ViktorT-11 added the no-changelog This PR is does not require a release notes entry label Sep 11, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @ViktorTigerstrom, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request is a foundational step in transitioning the firewalldb to a SQL-based system. It focuses on implementing the intricate migration process for 'action' records, ensuring that historical data is accurately transferred and correctly associated with related sessions and accounts. The current changes are primarily focused on the migration mechanism and its robust validation through testing, laying the groundwork for future production deployment.

Highlights

  • Action Data Migration: Introduced comprehensive logic to migrate 'action' data from the existing KV (BoltDB) store to the new SQL database within the firewalldb component. This includes handling various scenarios for linking actions to sessions and accounts.
  • New SQL Queries and Interfaces: Added new SQL queries and corresponding Go interfaces/implementations for GetAction (to retrieve action details from SQL) and UpdateAccountAliasForTests (a utility for testing account alias updates).
  • Extensive Test Coverage for Migration Logic: Significantly expanded the test suite for database migration, including numerous scenarios to validate the correctness of action migration. These tests cover cases with no linked entities, linked sessions, linked accounts, filtered entities, and complex scenarios involving multiple colliding entities.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This PR introduces the migration logic for actions from kvdb to SQL. The changes are extensive and include new SQL queries, migration logic, and a comprehensive set of tests. The migration logic correctly handles various scenarios of linking actions to sessions and accounts, with good heuristics for resolving ambiguities. The test suite is thorough, covering many edge cases. I've found a few issues, mostly minor comment discrepancies and a couple of bugs in the test code that prevent certain scenarios from being tested. Overall, this is a solid piece of work.

The upcoming commit will update the `AddActionReq` struct to include an
extra field which the `kvdb` actions store will ignore. Therefore the
`assertEqualActions` for the `kvdb` version will need to be update to
ignore this field. In preparation for that change, we also do another
optimization of the `assertEqualActions` function under kvdb builds, to
not mutate the passed action references.
When migrating the actions store from kvdb to sql, we will update the
existing actions to include the full mac root key, instead of just the
last 4 bytes (currently called `MacaroonIdentifier`). In order to do so,
we change the sql implementation of the `actions` store to persist the
full mac root key, instead of just the last 4 bytes. As no production
data in the sql actions store exists for users yet, it's fine for us to
change this without having to address old sql actions which only stored
the last 4 bytes.

Note though that we do not update the kvdb implementation, and the full
macaroon root key will be ignored by the kvdb store even if set.
Therefore, the rest of `litd` will still have to just expect the last 4
bytes of the mac root key when accessing an `Action`'s
MacaroonIdentifier. Therefore, we we currently never expose the rest of
the mac root key outside of the sql actions store.

Once the kvdb store has been fully deprecated and removed, we can then
update the rest of `litd` to also use the full mac root key, and change
the `Action` struct's field to reflect this.
Add a new SQL query `GetAction` to retrieve a single action by its ID.
This query will be needed for the kvdb to SQL migration of actions
store.
In the upcoming kvdb to SQL migration of the actions store, we need to
simulate in tests that two or more accounts have colliding account
aliases for the first 4 bytes of the alias. In order to allow creation
of such accounts, we need to be able to update the alias of an account
in tests, and this commit adds the a SQL query enabling this
functionality.

Note that the `UpdateAccountAliasForTests` query is only intended for
use in tests and should not be used in production code.
In preparation for the kvdb to SQL migration of the actions store, this
commit adds an `actions` field to the expected result of the migration
tests. Once the migration is implemented, this field will be used to
validate that the migrated actions match the expected results.
This commit adds an `accountStore` and a `rootKeyStore` arg the database
population functions of the kvdb to SQL migration tests of the
firewalldb.

As an action can be linked to an account, we need to enable simulation
of that in the migration tests of the actions store. In order to create
the accounts to link the actions to, we need to create the accounts in
the account store, which therefore requires passing the `accountStore`
to database population functions of the migration tests.

As the kvdb to SQL migration also will update the migrated actions to
not only store the 4 byte short ID of the action's corresponding
macaroon, but to it's full 8 byte root key ID. This requires the
migration function has access to all of lnd's 8 byte root key IDs, and
the migration function will therefore be change to accept a [][]byte
arg containing all of lnd's root key IDs.
As we can't access a full lnd instance in the migration unit tests, we
need to create a mock instance that simulates the root key store, and
this commit therefore adds mock `rootKeyStore` struct which is also
passed to the database population functions of the migration tests.
This `rootKeyStore` struct can be used to generate dummy root key IDs
when creating simulated actions in the migration tests.
This commit introduces the migration logic for transitioning the
actions store from kvdb to SQL.

Note that as of this commit, the migration is not yet triggered by any
production code, i.e. only tests execute the migration logic.
@ViktorT-11 ViktorT-11 force-pushed the 2025-07-migrate-actions branch from 2bd9a9c to 2682b91 Compare September 18, 2025 00:11
@lightninglabs-deploy
Copy link

@ViktorTigerstrom, remember to re-request review from reviewers when ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This PR is does not require a release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants